Skip to content

fix(worker): interactive terminal sessions hang when the upgrade stalls - #129

Open
MohammedAlkindi wants to merge 1 commit into
openclaw:mainfrom
MohammedAlkindi:fix/interactive-terminal-upgrade-timeout
Open

fix(worker): interactive terminal sessions hang when the upgrade stalls#129
MohammedAlkindi wants to merge 1 commit into
openclaw:mainfrom
MohammedAlkindi:fix/interactive-terminal-upgrade-timeout

Conversation

@MohammedAlkindi

@MohammedAlkindi MohammedAlkindi commented Sep 8, 2026

Copy link
Copy Markdown

Same defect class as #123-#128, in a file those did not reach.

What Problem This Solves

Resolves a problem where starting an interactive terminal session can hang indefinitely.
interactiveTerminalFetch issues the WebSocket upgrade handshake with no AbortSignal, so when the
upstream never completes the 101, the awaited call never settles and session setup wedges.

Why This Change Was Made

Bound the handshake with the same 10s timeout runtimeAdapterFetch uses two functions above in the same
file. This bounds the upgrade only: once the promise resolves with response.webSocket set, a later
abort is a no-op against an already-fulfilled promise, so established sessions are unaffected.

User Impact

A terminal session against an unresponsive runtime fails in about ten seconds instead of hanging until
the client gives up.

Evidence

  • fail-before (fix reverted, test kept): the new test fails with AssertionError: The expression evaluated to a falsy value; 4 pass, 1 fail, nothing else in the file breaks.
  • pass-after: tests/runtime-adapter-transport.test.ts 5/5.
  • pnpm test: 1004 tests, 1000 pass, 4 fail. A clean checkout gives 1003 / 999 / 4 with an identical
    failing set, so this adds one passing test and no new failures.

The test asserts the signal as this file already does for runtimeAdapterFetch, rather than waiting out
a real abort.

Could not verify: pnpm check does not complete on my Windows machine, on this branch or on a clean
checkout. scripts/generate-assets.mjs spawns vite as /C:/Users/.../vite/bin/vite.js, which Node
resolves to C:\C:\Users\... and fails MODULE_NOT_FOUND. It reproduces without my change, so lint and
format are unproven from here.

@MohammedAlkindi
fix(worker): interactive terminal sessions hang when the upgrade stalls
906d0ad 
interactiveTerminalFetch issues the WebSocket upgrade handshake with no
AbortSignal, so if the upstream never completes the 101 the awaited call
never settles and terminal session setup wedges indefinitely.

Bound it with the same 10s timeout runtimeAdapterFetch already uses two
functions above in the same file. This bounds only the handshake: once
the promise resolves with response.webSocket set, a later abort is a
no-op against an already-fulfilled promise, so established sessions are
unaffected.
@clawsweeper

clawsweeper Bot commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 8, 2026
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 8, 2026, 3:30 AM ET / 07:30 UTC.

ClawSweeper review

What this changes

Adds a ten-second timeout to upstream terminal WebSocket requests, a signal-presence test, and an Unreleased changelog entry.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 7 items remain

The timeout is still needed, but the proposed implementation also aborts healthy terminal connections after ten seconds. The submitted tests do not exercise this runtime behavior.

Priority: P2
Reviewed head: 906d0ad0ffb624ee5337cc639487872a42b79d31

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused fix addresses a real gap, but its cancellation lifetime breaks healthy terminals and the current proof cannot detect that regression.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The submitted recording-fetcher test checks signal presence but never exercises interactiveTerminalFetch through a real Worker WebSocket transport; stalled-handshake cancellation and continued established-session I/O need after-fix evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The submitted recording-fetcher test checks signal presence but never exercises interactiveTerminalFetch through a real Worker WebSocket transport; stalled-handshake cancellation and continued established-session I/O need after-fix evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Verified scope and policy: The remote identifies openclaw/crabfleet. The full root AGENTS.md was read; no nested AGENTS.md or maintainer-notes directory was found. The introduced diff contains exactly the transport change, its test, and a changelog entry. Required build/test gates were not executed under this read-only review.
Current main and release still lack the deadline: Both fetched main and v0.3.1 call the terminal fetcher with headers alone; the nearby runtimeAdapterFetch timeout does not cover this separate call. No merged replacement was established.
Upgraded socket remains in active use: openUpstream awaits interactiveTerminalFetch, accepts response.webSocket, and returns that socket for ongoing terminal traffic. TerminalHub already catches connection failures and sends a redacted error to the client.
Findings 1 actionable finding [P1] Clear the deadline after the WebSocket handshake settles
Security None None.

How this fits together

Crabfleet’s Worker connects browser terminal subscriptions to runtime-provider WebSockets. This transport selects the coordinator binding or network fetch, then hands the upgraded socket to the terminal relay.

flowchart LR
  A[Browser terminal subscription] --> B[Terminal service]
  B --> C[Select upstream transport]
  C --> D[Coordinator binding]
  C --> E[Network fetch]
  D --> F[WebSocket handshake]
  E --> F
  F --> G[Terminal relay or connection error]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The submitted recording-fetcher test checks signal presence but never exercises interactiveTerminalFetch through a real Worker WebSocket transport; stalled-handshake cancellation and continued established-session I/O need after-fix evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Clear the deadline after the WebSocket handshake settles (P1) - Cloudflare does not detach this signal when fetch resolves: the pinned workerd runtime wraps the successful 101 socket in AbortableWebSocket using the signal’s canceler (src/workerd/api/http.c++:1655). Consequently, a healthy terminal that connects promptly is aborted ten seconds after the request starts. Use an AbortController and a timer cleared in finally around await fetcher.fetch(...), and cover continued socket I/O beyond the deadline as well as a stalled handshake.
  • Resolve merge risk (P1) - Merging this implementation would disconnect healthy provider terminals when the ten-second signal fires.
  • Resolve merge risk (P1) - Actual stalled-upgrade cancellation and successful terminal operation beyond the deadline remain unverified.
  • Complete next step (P2) - Repair the timer lifetime and add real Worker transport proof for timeout and continued terminal I/O. Redacted logs or terminal output are sufficient; remove credentials and private endpoints. Update the PR body to trigger re-review, or ask a maintainer for @clawsweeper re-review if needed.
  • Improve patch quality - Replace the persistent timeout signal with a handshake timer cleared on success and failure.
  • Improve patch quality - Add redacted Worker-runtime evidence showing a stalled upgrade times out and an established terminal exchanges data beyond ten seconds.

Findings

  • [P1] Clear the deadline after the WebSocket handshake settles — src/worker/runtime-adapter-transport.ts:72
Agent review details

Security

None.

Review metrics

None.

Merge-risk options

Maintainer options:

  1. Limit cancellation to the handshake (recommended)
    Clear a cancellable deadline when fetch settles and demonstrate that established terminal traffic continues beyond ten seconds.

Technical review

Best possible solution:

Use an AbortController with a ten-second timer cleared in finally after awaiting the upgrade, preserving established sockets while bounding stalled handshakes.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes both the unbounded main-branch handshake and the patch’s cancellation of established sockets; neither scenario was executed during this read-only review.

Is this the best way to solve the issue?

No. AbortSignal.timeout remains effective after a Cloudflare WebSocket upgrade; a timer cleared when the awaited handshake settles is the narrow repair.

Full review comments:

  • [P1] Clear the deadline after the WebSocket handshake settles — src/worker/runtime-adapter-transport.ts:72
    Cloudflare does not detach this signal when fetch resolves: the pinned workerd runtime wraps the successful 101 socket in AbortableWebSocket using the signal’s canceler (src/workerd/api/http.c++:1655). Consequently, a healthy terminal that connects promptly is aborted ten seconds after the request starts. Use an AbortController and a timer cleared in finally around await fetcher.fetch(...), and cover continued socket I/O beyond the deadline as well as a stalled handshake.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against bdd5083b0d3d.

Labels

Label changes:

  • add P2: This addresses a bounded runtime-provider terminal hang without evidence of a current widespread outage.
  • add merge-risk: 🚨 availability: The introduced timer remains attached to the upgraded WebSocket and would terminate otherwise healthy terminal connections.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The submitted recording-fetcher test checks signal presence but never exercises interactiveTerminalFetch through a real Worker WebSocket transport; stalled-handshake cancellation and continued established-session I/O need after-fix evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This addresses a bounded runtime-provider terminal hang without evidence of a current widespread outage.
  • merge-risk: 🚨 availability: The introduced timer remains attached to the upgraded WebSocket and would terminate otherwise healthy terminal connections.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The submitted recording-fetcher test checks signal presence but never exercises interactiveTerminalFetch through a real Worker WebSocket transport; stalled-handshake cancellation and continued established-session I/O need after-fix evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Verified scope and policy: The remote identifies openclaw/crabfleet. The full root AGENTS.md was read; no nested AGENTS.md or maintainer-notes directory was found. The introduced diff contains exactly the transport change, its test, and a changelog entry. Required build/test gates were not executed under this read-only review. (AGENTS.md:3, 906d0ad0ffb6)
  • Current main and release still lack the deadline: Both fetched main and v0.3.1 call the terminal fetcher with headers alone; the nearby runtimeAdapterFetch timeout does not cover this separate call. No merged replacement was established. (src/worker/runtime-adapter-transport.ts:72, 8cd813822924)
  • Upgraded socket remains in active use: openUpstream awaits interactiveTerminalFetch, accepts response.webSocket, and returns that socket for ongoing terminal traffic. TerminalHub already catches connection failures and sends a redacted error to the client. (src/worker/interactive-terminal-service.ts:202, 906d0ad0ffb6)
  • Affirmative runtime dependency: The changed fetch uses Cloudflare’s WebSocket response extension, and the lockfile pins workerd 1.20260730.1 through the Worker tooling; its fetch cancellation contract directly governs the PR’s claim that established sockets are unaffected. (pnpm-lock.yaml:1501, 906d0ad0ffb6)
  • Fetch cancellation survives a successful upgrade: At tag v1.20260730.1, fetch wraps the successful 101 WebSocket in AbortableWebSocket using the supplied signal’s canceler before resolving the response. Therefore resolving the fetch promise does not detach cancellation from the socket. (src/workerd/api/http.c++:1655, 26b5461b7dcc)
  • Abort destroys the underlying connection: AbortableImpl registers a cancellation listener that releases its underlying object; WebSocket send and receive operations use this canceler. The unconditional ten-second signal consequently terminates established terminal I/O. (src/workerd/util/abortable.h:18, 26b5461b7dcc)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MohammedAlkindi